Claude finished @lwalew's task in 3m 59s —— View job

Review: inference_speed benchmark

Nice, well-documented benchmark — clean separation of the two throughput metrics (engine-independent forward pass vs. end-to-end MD), thorough docstrings, license headers, docs, and CHANGELOG all present. However, there's a blocking bug that means the benchmark cannot run (or be analyzed) as written.

🔴 Blocking: self._dataset_name is never defined → AttributeError

inference_speed.py defines a class attribute dataset_name = "scaling" (line 177) but then reads self._dataset_name (note the leading underscore) in two places:

• run_model → src/mlipaudit/benchmarks/inference_speed/inference_speed.py:194
• _structure_filenames → inference_speed.py:445 (reached from analyze via _structure_names)

_dataset_name is not defined anywhere — not as a class attribute, not on the base Benchmark, and not in mocked_benchmark_init (which only patches _download_data). So any call to run_model() or analyze() raises AttributeError: 'InferenceSpeedBenchmark' object has no attribute '_dataset_name'. The test tests/inference_speed/test_inference_speed.py:56 (assert ..._dataset_name == "scaling") hits the same error.

The root cause is that benchmark.py was not modified in this PR (no diff vs develop), yet the CHANGELOG claims "Benchmarks can now reuse another benchmark's dataset via the dataset_name attribute." The base class already provides this via data_name + the data_dir property (benchmark.py:112, :235-241), and dataset_name/_dataset_name are not wired into it at all. A second consequence: because data_name is left unset, _download_data (benchmark.py:244) will try to fetch inference_speed.zip from HuggingFace — which does not exist — instead of reusing scaling.

Suggested fix — use the existing base-class mechanism rather than inventing a new attribute:

# class body
data_name = "scaling"          # instead of `dataset_name = "scaling"`

# run_model (line ~194)
atoms = ase_read(self.data_dir / f"{structure_name}.xyz")

# _structure_filenames (line ~445)
os.listdir(self.data_dir)

and update the test to assert benchmark.data_name == "scaling" (or benchmark.data_dir.name == "scaling"), plus the docstring/CHANGELOG wording.

Fix this →

Note: the CI status comment above (133 passing) is dated 2026-06-26, but both commits on this branch are dated 2026-07-09, so that green check does not cover the current code. Worth re-running the suite.

🟠 Minor: early-failure return omits required structures field

inference_speed.py:383-386:

if len(self.model_output.simulation_states) == 0:
    return InferenceSpeedResult(structure_names=self._structure_names, failed=True)

InferenceSpeedResult.structures has no default, so this raises a Pydantic ValidationError on the empty-run path. (ScalingResult has the same latent issue, so this is copied behaviour rather than new — but worth fixing here by passing structures=structure_results.)

🟡 Minor: fit lines ignore the per-structure timestep

_build_fit_lines (src/mlipaudit/ui/inference_speed.py:192) converts fitted times with DEFAULT_TIMESTEP_FS (1.0), while the plotted points use each structure's actual timestep_fs (inference_speed.py:125). For the MD ns/day metric these will disagree if a result was ever recorded with a timestep ≠ 1 fs, offsetting the fit line from its points. Today all configs use 1 fs so the impact is nil, but it's an inconsistency — consider threading the structures' timestep through, or documenting the assumption.

✅ Looks good

• NS_PER_DAY_FACTOR = 0.0864 (= 86400 × 1e-6) and the ns/day conversion are correct.
• Per-atom normalisation in _compute_score is sound; the placeholder t₀ is clearly flagged with a TODO and the hardware-relative caveat is well documented.
• mlip vs. ASE forward-pass timing (pre-built graph + jax.block_until_ready, vs. all_changes recompute + result read for device sync) is a reasonable, clearly-commented distinction.
• Logging via the mlipaudit logger, Google-style docstrings, license headers, UI/API/benchmark docs, and CHANGELOG entry are all in place.

Once the _dataset_name issue is resolved and the suite re-run, this is close to mergeable.

Claude finished @lwalew's task in 2m 7s —— View job

Re-review: inference_speed benchmark

Thanks for the fixes — I re-reviewed the current head (626480f) against the previously-flagged issues.

• Verify the blocking _dataset_name bug is fixed
• Check the early-failure structures field issue
• Check the fit-line timestep inconsistency
• Fresh pass over new/changed code
• Post review

✅ Resolved since last review

• Blocking _dataset_name AttributeError — fixed. The class now sets data_name = "scaling" (inference_speed.py:177) and both run_model (:193) and _structure_filenames (:445) read from the base-class self.data_dir property. No _dataset_name references remain anywhere, and data_name = "scaling" correctly routes _download_data/data_dir at benchmark.py:241-246 to reuse the scaling dataset. Test updated to assert data_name/data_dir.name (test_inference_speed.py:56-57). 👍
• Early-failure ValidationError — fixed. The empty-run branch now passes structures=structure_results (inference_speed.py:381-386), so InferenceSpeedResult.structures is populated.

🟠 CHANGELOG: duplicated entry + wrong attribute name

The CHANGELOG diff adds the inference_speed bullet twice — once under the new ## Unreleased section (correct) and once under ## Release 0.1.5, which is an already-released version (CHANGELOG.md:27-34). New work shouldn't be appended to a shipped release section. Additionally:

- Benchmarks can now reuse another benchmark's dataset via the `dataset_name` attribute.

This line (CHANGELOG.md:34) names a dataset_name attribute that does not exist — the mechanism is the data_name attribute + data_dir property, which is already documented by the existing 0.1.5 bullet "Add a data_name attribute (and data_dir property)…". So this line is both incorrectly named and redundant.

Suggested fix: delete the entire duplicated block from the ## Release 0.1.5 section (the second inference_speed bullet and the dataset_name line), keeping only the ## Unreleased entry.

Fix this →

🟡 Minor: fit lines still ignore the per-structure timestep

_build_fit_lines (src/mlipaudit/ui/inference_speed.py:192) still converts fitted times with DEFAULT_TIMESTEP_FS (1.0), whereas the plotted points use each structure's actual timestep_fs (inference_speed.py:125). For the MD throughput (ns/day) metric the fit line and its points would diverge if a result were ever recorded with a timestep ≠ 1 fs. All current configs use 1 fs so the impact is nil today — worth threading the structures' timestep through the fit, or dropping a short comment noting the assumption. (Unchanged from the prior review; flagging as still-open, not blocking.)

🟡 Nit: PR description is now stale

The PR body still describes the speed score as a Hill function on the per-atom step time (MD). The code now scores the per-atom model forward time (inference_speed.py:429-440), which is the better, engine-independent choice — the in-code docstrings correctly reflect this. Just update the PR summary so it matches.

Notes

• I couldn't run the test suite here (pytest needs approval in this environment), so I verified the fixes by reading. The last green CI comment (133 passing) predates these commits — worth confirming CI is green on the current head, especially tests/inference_speed.
• The data_name = "scaling" reuse relies on scaling.zip existing on HuggingFace and scaling never being renamed; that's an acceptable coupling and is documented in the docstring.

Once the CHANGELOG duplicate/naming is cleaned up, this looks good to merge. The remaining fit-line item is cosmetic under current configs.